Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ECP-765: GraphQL Schema for Configurable Options Selection #394

Merged
merged 5 commits into from
Jul 17, 2020

Conversation

paliarush
Copy link
Contributor

@paliarush paliarush commented Jul 8, 2020

Edit by @DrewML: Added description from JIRA ticket for reviewers

Design GraphQL schema to address issue related to thousands of configurable option permutations.

It should allow to avoid fetching all variants on configurable product page and instead allow incremental retrieval of options available for selection. It should also be possible to add product to cart when all options are selected (simple product representing variant should be identifiable).

@@ -0,0 +1,14 @@
type Query {
configurableOptionsSelectionMetadata(configurableProductSku: ID!, selectedConfigurableOptionValues: [ID!]): ConfigurableOptionsSelectionMetadata @doc(description: "Get metadata for the specified configurable options selection")
Copy link
Contributor

@DrewML DrewML Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Configurable options can't exist outside of a product, right?

Seems like this might make more sense as a field under ConfigurableProduct? Right now it seems like we're namespacing via naming convention instead of type.

One benefit to moving to the ConfigurableProduct type is that the client doesn't need to know the configurableProductSku ahead of time - this covers the case where the UI wants to render in a single request with default selections, and doesn't know the sku before the request (UI fetches product using a url_key filter with eq matcher in this scenario).

query ConfigurableOptionsAvailable($url_key: ID!, $selectedOptions: [ID!]!) {
   products(filter: { url_key: { eq: $url_key } }) {
	 items {
        name
        description
        sku
        ...on ConfigurableProduct {
           options_selections(selectedOptions: $selectedOptions) {
              # select fields
           }
        }
     }
   }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say this depends on whether we want to support this functionality in category/search or not, discussed below #394 (comment)

Copy link
Contributor

@DrewML DrewML Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth keeping in mind, you can get a link to a product page with pre-selected options, so I don't think this is just about category/search support.

For example, the merchant we discussed on the call yesterday includes selected option IDs in the URL for their product pages. If that's linked to someone, when they load the page the UI should be able to get the new selection set, without requiring a second round-trip (and client doesn't have sku from the URL itself, but does have url_key).

Example URL: https://www.mystore.com/url-key?selectedOptionID=4&selectedOptionValueID=16

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be two calls in my understanding:

  1. urlResolver
  2. Combined query: product details (using new query by ID/sku) + available selections query

URL will have to contain selections in compatible format, that is currently used in add to cart with single mutation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity: This was resolved via a change made to address a different comment. 2 birds, 1 stone 😄


After the user makes final selection, the corresponding simple product data becomes available and the product can now be added to cart.

### Render configurable option values available for selection on the category page
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Category and search result pages do not require the list of configurable option values available for selection

I think this might be an unsafe assumption. In a headless world we want to avoid limiting use-cases, especially ones that would allow a shopper to get a product into the cart faster.

Is the idea to not support that at all, or that folks would have to go back to using variants field for options when rendering a category?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not possible to provide selections in category/search queries, unless I am missing something.

This is mainly the reason why I decided to have a dedicated query for option selections instead of making them a field of configurable product. The only way to use them would be with empty selections, which does not make a lot of sense because we already have access to full set of option values (not only those available for selection, but all of them).

Copy link
Contributor

@DrewML DrewML Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not possible to provide selections in category/search queries, unless I am missing something.

Not quite talking about including it in the query used for rendering the results page. Here is the general use-case I'm talking about:

  1. Shopper goes to store, and clicks on "Jeans" in the nav menu.
  2. Category page for "Jeans" renders, with product tiles representing each product
  3. Each product tile renders some color options (Raw/Acid Wash/Light Wash) and size options (Small/Medium/Large). These are rendered as button elements
  4. Shopper clicks "Acid Wash" on the product tile - that product tile should be able to fetch and limit the "Size" options now

As a shopper this is an experience familiar to me on ecommerce websites, and as a front-end dev it's an experience I'd like the API to make possible.

FWIW, I'm not saying your API doesn't support that, more just commenting on us explicitly documenting that we don't want to support this flow, which I think is fairly typical for ecomm.


It is not possible to provide selections in category/search queries, unless I am missing something.

Do we expect this to always be true? I'd assume that a search service would support faceted search (cc @kokoc), and I believe facets are driven by attributes? If a search filtered to only include the "Color" attribute with value "Green," I think the client should ideally be able to pre-select that and get back the other options after color selection (like available sizes).

Copy link
Contributor Author

@paliarush paliarush Jul 8, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the category use case agree, we can clarify that your use case is supported by the new query. What I meant is that product listing query itself should not support fetching available selections, because all possible selections can be displayed there.

Regarding faceted search, this information may be available in the facet section. Again I don't see a use case why and how we can implement it on the level of the specific product returned in search results. To me this is the same use case as for category and I would not separate them.

Copy link
Contributor

@DrewML DrewML Jul 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again I don't see a use case why and how we can implement it on the level of the specific product returned in search results. To me this is the same use case as for category and I would not separate them.

It's likely I'm explaining the facet filtering poorly, but I do think this is a separate use-case. Let me try and expand more.

image

Imagine that this screenshot of Luma is from a headless store built entirely against GraphQL.

I'm a shopper and I open up the filters on the left and choose "Red." This will limit my results to shorts that come in red, but the result set will still have other colors, too.

At this point, the UI knows that I'm trying to drill down to get a red product in my cart. A sufficiently smart UI should now be able to pre-select "red" on each tile for me, s the sizes displayed on each tile are limited to those available in red (because red should be selected).

Keeping the query attached to the ConfigurableProduct type enables this flow. Imagine this example query:

query FacetedCategoryResults($categoryID: ID, $selectedOptionIDs: [ID!]!) {
   # Note: Only selecting one category here, but looks weird cause we don't have
   #       a root query to return a single category
   categories(filters: { ids: [$categoryID] }) {
      items {
		...on ConfigurableProduct {
           # Field available on ConfigurableProduct type allows us to do this in 1 query
           options_values(selectedOptionIDs: $selectedOptionIDs) {
              # pick out fields needed for UI
           }
        }
      }
   }
}

If we make available options for a selection only available as a root query, the client would have to do N+1 requests to enable this same flow.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for explanation, now your use case is clear.

In this case we will have to think how it will work in other scenarios involving ConfigurableProduct type, like search and other product listing queries. The question there is how should it work when products in the result belong to different attribute sets and thus have different configurable options.

@nrkapoor do we want to go this route to support additional use cases like Andrew described above?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, have to give credit for the use-case to @soumya-ashok. She had this idea for pwa-studio early on and I thought it was pretty cool!

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DrewML Thanks for remembering and surfacing the idea we'd discussed. Additionally, if the listing is clicked on to go the PDP, the applied color filter could be the automatic selection.

The same should apply for size or any other filtered parameter.

This should be the more common pattern in Commerce experiences, but surprisingly isn't!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed

@paliarush paliarush changed the title [DRAFT] ECP-765: GraphQL Schema for Configurable Options Selection ECP-765: GraphQL Schema for Configurable Options Selection Jul 8, 2020
Copy link
Contributor

@nrkapoor nrkapoor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DrewML @paliarush It makes sense to cover additional use cases as well so we minimize future iterations.

{
configurable_options_available_for_selection: [ConfigurableOptionAvailableForSelection!] @doc(description: "Configurable options available for further selection based on current selection.")
media_gallery: [MediaGalleryInterface!] @doc(description: "Product images and videos corresponding to the specified configurable options selection.")
variant: SimpleProduct @doc(description: "Variant represented by the specified configurable options selection. It is expected to be null, until selections are made for each configurable option.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paliarush should this be an array of simple products? Also, is it possible to have a virtual/bundled/grouped product as a variant?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check @doc, if after that it is still not clear, we need to improve it.

Can configurable variant be of any types other than simple?

Copy link
Contributor

@nrkapoor nrkapoor Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paliarush assuming we can still get to a simple product using default values for all unselected options. Use case: a color selection from the swatch should change the image on the UI. cc: @DrewML

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If they are default, client will explicitly pass them to GraphQL.

Copy link
Contributor

@DrewML DrewML Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@paliarush assuming we can still get to a simple product using default values for all unselected options. Use case: a color selection from the swatch should change the image on the UI. cc: @DrewML

@nrkapoor my understanding is that Magento 2 itself does not have a concept of a "default value" for configurable options. Basically, the client will be able to get a list of all options available based on current selections, and it'll be up to the client to choose the strategy for "default", based on the data they have.

So this schema does unlock that feature, but it'll be up to the client to define some of their own logic around it.

There are extensions in the Marketplace that add defaults - I think they'll be able to extend our schema to support that (guessing they'd add a field to ConfigurableOptionAvailableForSelection).


type ConfigurableOptionsSelectionMetadata @doc(description: "Metadata corresponding to the configurable options selection.")
{
configurable_options_available_for_selection: [ConfigurableOptionAvailableForSelection!] @doc(description: "Configurable options available for further selection based on current selection.")
Copy link
Contributor

@DrewML DrewML Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can make configurable_options_available_for_selection a bit more concise without losing meaning.

Because this field lives inside the ConfigurableOptionsSelectionMetadata type, and is being plucked from a field named configurable_options_selection_metadata , seems like the configurable_options prefix is a bit redundant.

{
   ...on ConfigurableProduct {
     configurable_options_selection_metadata(id: $ID) {
		configurable_options_available_for_selection {
         # Can drop the prefix on the field without losing meaning
        }
     }
   }

Copy link
Contributor

@DrewML DrewML Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about something like this?

Not married to the specific names, but I think the query is a bit more self-describing on first read.

Another small benefit is that the client-side doesn't have to keep repeating configurable_options_something_something everywhere the object is passed around (or alias field usage every time).

query GetUpdatedOptionsValues($sku: String!, $selectedOptionIDs: [ID!]) {
    products(filter: {sku: {eq: $sku}}) {
        ...on ConfigurableProduct {
            availableOptions(selectedOptionIDs: $selectedOptionIDs) {
                optionGroups {
                    attribute_code
                    available_value_ids
                }
            }
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe having "selection metadata" is very important. Because it not only contains available options, but selection metadata like variant, price range, low stock status etc.

"configurable options" prefix is added for consistency with other product type specific fields like configurable_options.

I'm okay with renaming configurable_options_available_for_selection to options_available_for_selection since it is located under configurable_options_selection_metadata. Reducing it further will hurt the meaning I think.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"configurable options" prefix is added for consistency with other product type specific fields like configurable_options.

That's reasonable. The repetition is a bit annoying for a client, but the trade-off is that we're less likely to have field name collisions down the line, which we've already learned is a real problem 😅

I'm okay with renaming configurable_options_available_for_selection to options_available_for_selection since it is located under configurable_options_selection_metadata. Reducing it further will hurt the meaning I think.

Cool! Let's go with that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -0,0 +1,20 @@
type ConfigurableProduct {
configurable_options_selection_metadata(selectedConfigurableOptionValues: [ID!]): ConfigurableOptionsSelectionMetadata @doc(description: "Metadata for the specified configurable options selection")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar Comment

Maybe we can drop the configurable prefix here? If you're working with a ConfigurableProduct, you typically are fetching it from a field using ProductInterface, so a client query already has to include ...on ConfigurableProduct.

Based on that, I think we can drop the configurable_ prefix, because it's assumed that a configurable product has configurable options.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Se response in another related comment #394 (comment)

User navigates to the configurable product page. Option values available for selection are rendered on the page.

```graphql
{
Copy link
Contributor

@DrewML DrewML Jul 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been looking at this query for a bit, and trying to think through how folks like @magento/pwa-studio-team are going to consume this and render it in a UI.

It looks like the UI will do something like this:

  • Loop over data.products.items[0].configurable_options
    • For each option, grab attribute_code
    • Loop over configurable_options_selection_metadata.configurable_options_available_for_selection for the matching attribute_code from the outer loop

That nested looping is necessary because configurable_options_selection_metadata is disconnected from configurable_options, and is sort-of weakly referenced via an ID, leaving it to the UI to go find that object.

What would the impact be if we moved configurable_options_selection_metadata into ConfigurableProductOptions?

Idea 1

With this model, clients always get back a list of all option values, and can do their own filtering to decide whether they're shown or not. This supports the UX where a user can see a green shirt has a size "small," but it's not available.

{
    products(filter: {sku: {eq: "sku-here"}}) {
        ...on ConfigurableProduct {
            configurable_options(selectedOptionValueIDs: [ID!]) {
                attribute_code
                label
                values {
                   id # ID can be checked against `available_value_ids`
                }
                available_value_ids
            }
        }
    }
}

Idea 2

With this model, client can either select a list of all option values, or stick to just the values currently available to the client. This supports the UX where options that aren't available just aren't rendered.

{
    products(filter: {sku: {eq: "sku-here"}}) {
        ...on ConfigurableProduct {
            configurable_options(selectedOptionValueIDs: [ID!]) {
                attribute_code
                label
                # Client can select this if they want a UI that shows greyed-out, unavailable options
                values {
                   id
                }
                # Same return type as `values` field, but limited to just values allowed.
                # Useful for UIs that don't render unavailable options at all
	            availableValues {
                   id
                }
            }
        }
    }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As mentioned in other comments above, in this case we are loosing the "Metadata" aspect and extensibility.

My understanding was that on the first request (with no selections made), the client will only be using configurable_options field. We can add availability flag to the values there, it makes sense in my opinion.
The subsequent queries only ask for options_available_for_selection and probably other metadata like variant, price_range etc. They already have a list of all options with values from the initial request and do not need to query configurable_options anymore.

Copy link
Contributor

@DrewML DrewML Jul 13, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding was that on the first request (with no selections made), the client will only be using configurable_options field

This is my understanding as well, so far on the same page 👍

We can add availability flag to the values there, it makes sense in my opinion

Cool 👍 I like the idea.

They already have a list of all options with values from the initial request and do not need to query configurable_options anymore.

If some fields aren't necessary on the second request (attribute_code/values/label) the client just won't select those fields. I don't think that's necessarily a reason to split these data types out into sibling fields vs nested (configurable_options vs configurable_options_selection_metadata ).

in this case we are loosing the "Metadata" aspect and extensibility.

It's Monday and I'm not running on all cylinders yet 😅 - can you give an example of the extensibility we lose this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added is_available_for_selection flag to option value type.

Regarding extensibility, I mean that the fields like price_range, variant, low_stock_status will not fit into existing configurable_options field. While they look really good in configurable_options_selection_metadata in my opinion.

Copy link
Contributor

@DrewML DrewML Jul 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While they look really good in configurable_options_selection_metadata in my opinion.

My biggest concern is still around how ergonomic this is for UI developers to consume, which is the API's primary audience.

In a typical modern front-end, things are broken up by components, and you just describe how you want the UI to look by composing those components.

Let's step a bit back from this specific schema, and talk more in terms of how the UI code would be structured.

In this case, the UI code would ideally just want to loop over the options groups (color/size/etc), and then render the options with that dataset.

Note: Code below uses less abstract names than our schema, to try and make the example as clear as possible

// Something like this using JSX, but pretty similar in a template-based world like Vue
const options = option_groups.map(option => 
   <OptionGroup
     attrCode={option.attribute_code}
     label={option.label}
     values={option.values}
   />
);

Then, the <OptionGroup /> component would just iterate over values, and spit out a list of HTML elements representing all the values in the option group. Ideally, option.values here would have data about whether or not the option is selectable, so it can be greyed-out, or sorted to move available options to the front of the list.

With the currently proposed schema, if the UI wanted to do this, it would have to either have to normalize data into a lookup table every time a response comes in, or do something like this:

const options = options_groups.map(option =>
   <OptionGroup
     attrCode={option.attribute_code}
     label={option.label}
     values={option.values}
     // This nested loop could be several hundred iterations for each option group,
     // and just isn't super fun to write
     availableValues={options.find(o => o.attribute_code === option.attributeCode)}
   />
);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having said all of that, if you can't see an alternative schema design working, we can always start with this and change course or add later if we find usability problems.


# Configurable option value type has to be extended to include ID which can be used to uniquely identify the option value across the system. This is consistent with proposal of single mutation for add-to-cart
type ConfigurableProductOptionsValues {
id: ID!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to verify, this ID includes a SKU in it so there won't be collisions, right?

Want to make sure 2 products can't create the same ConfigurableProductOptionsValues.id with different values for ConfigurableProductOptionsValues.is_available_for_selection

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it should, based on discussions in other PR related to IDs.

@kokoc
Copy link
Member

kokoc commented Jul 14, 2020

Agree with the necessity to have a separate query/node responsible for progressive options load.

There were parallel discussions about product structure on the storefront. Here are some outcomes:

  • We want to unify different type of options in "add to cart" scenario. This includes selected configurable product, selected downloadable link, selected custom options, etc. This mutation accepts selected options
  • We thought about the unification of options on PDP page as well, so custom options, downloadable options could be a part of the same container as configurable options.
  • We agreed on the following wording
    • Option - a choice customer may make on UI. E.g. any dropdowns, checkboxes, etc. This includes configurable options, downloadable options, custom options, etc.
    • Variation - more physical thing which is defined by set of selected options. Includes price, sku, qty, etc.

We also thought about two queries:

  • Accepts current selection of options and returns other available options as a result. Empty selection is passed on the first request.
  • When all required options are selected or the first query returns an empty result, the second query should be executed. This query accepts full selection only and provide variation(sku, prices, etc) as a result.

Overall our idea is to apply the proposal you have on more product types and make it a first-level citizen instead of product types.

@akaplya @melnikovi

@DrewML
Copy link
Contributor

DrewML commented Jul 14, 2020

When all required options are selected or the first query returns an empty result, the second query should be executed

@kokoc This is more of a REST-style way of doing things. It's typically a smell with a GraphQL API anytime you need to run up to the server to check if something has a value before you run a separate query. Should be achievable in a single round-trip

@nrkapoor
Copy link
Contributor

Agree with the necessity to have a separate query/node responsible for progressive options load.

There were parallel discussions about product structure on the storefront. Here are some outcomes:

  • We want to unify different type of options in "add to cart" scenario. This includes selected configurable product, selected downloadable link, selected custom options, etc. This mutation accepts selected options

  • We thought about the unification of options on PDP page as well, so custom options, downloadable options could be a part of the same container as configurable options.

  • We agreed on the following wording

    • Option - a choice customer may make on UI. E.g. any dropdowns, checkboxes, etc. This includes configurable options, downloadable options, custom options, etc.
    • Variation - more physical thing which is defined by set of selected options. Includes price, sku, qty, etc.

We also thought about two queries:

  • Accepts current selection of options and returns other available options as a result. Empty selection is passed on the first request.
  • When all required options are selected or the first query returns an empty result, the second query should be executed. This query accepts full selection only and provide variation(sku, prices, etc) as a result.

Overall our idea is to apply the proposal you have on more product types and make it a first-level citizen instead of product types.

@akaplya @melnikovi

@akaplya @paliarush @DrewML is this something we have already done with Add to Cart single mutation design?

@kokoc
Copy link
Member

kokoc commented Jul 14, 2020

When all required options are selected or the first query returns an empty result, the second query should be executed

@kokoc This is more of a REST-style way of doing things. It's typically a smell with a GraphQL API anytime you need to run up to the server to check if something has a value before you run a separate query. Should be achievable in a single round-trip

You are right, something like union type could be used in one query instead of multiple queries

@akaplya akaplya merged commit 4fdaee6 into master Jul 17, 2020
@@ -0,0 +1,21 @@
type ConfigurableProduct {
configurable_options_selection_metadata(selectedConfigurableOptionValues: [ID!]): ConfigurableOptionsSelectionMetadata @doc(description: "Metadata for the specified configurable options selection")
Copy link

@paales paales Jul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was pretty hard to determine what the API’s should do. The naming didn’t make it clear what the configurable_options_selection_metadata will return.

It seems the return value of configurable_options_selection_metadata is some form of aggregate of the remaining possible simples?

metadata is a very generic word? Maybe omit that? configurable_options_selection

Not sure if this is the best name or we can figure out something more descriptive?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Paul, thanks for reviewing. We can extend description of the field. Please check https://github.com/magento/architecture/pull/394/files?short_path=567352f#diff-567352fc8df3d0073eac2ea540e6f57f and let me know what would you like to include in the @doc. I think that document also answers your questions regarding extensibility and price range.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, regarding metadata in the name. Selection is provided as an input, so we are getting metadata for the provided selection.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Selection is provided as an input, so we are getting metadata for the provided selection.

Yes, that is true that it returns some form of metadata, but following that logic you can also call it: configurable_options_selection_metadata_data_aggregate Which still is true, but doesn't add any value?

In my opinion the word metadata doesn't add any value so it can be omitted.

Comment on lines +5 to +10
type ConfigurableOptionsSelectionMetadata @doc(description: "Metadata corresponding to the configurable options selection.")
{
options_available_for_selection: [ConfigurableOptionAvailableForSelection!] @doc(description: "Configurable options available for further selection based on current selection.")
media_gallery: [MediaGalleryInterface!] @doc(description: "Product images and videos corresponding to the specified configurable options selection.")
variant: SimpleProduct @doc(description: "Variant represented by the specified configurable options selection. It is expected to be null, until selections are made for each configurable option.")
}
Copy link

@paales paales Jul 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Shouldn’t is_available_for_selection also be available here? If I select a certain color it can be that resulting sizes aren’t all selectable? That is available bij the available_value_ids
  • Shouldn't price_range be available here as well or do you expect to that recalculation on the client (seems cumbersome)?

I like the possible extension point here to add delivery date information, etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's discuss in the comments above since it is all related: #394 (comment)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow how that comments answers the price_range question?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants